refactor(sdk): introduce NonZeroIggyDuration for retries and heartbeats - #3891

Codecov Report

❌ Patch coverage is 84.89011% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.80%. Comparing base (2b52fe0) to head (176e7a1).
⚠️ Report is 1 commits behind head on master.

@@             Coverage Diff              @@
##             master    #3891      +/-   ##
============================================
- Coverage     83.84%   83.80%   -0.04%     
  Complexity     1358     1358              
============================================
  Files          1212     1212              
  Lines        166831   167046     +215     
  Branches     134305   134658     +353     
============================================
+ Hits         139876   140000     +124     
+ Misses        23314    23258      -56     
- Partials       3641     3788     +147     

/request-review @hubcio

/ready

The problem is real for consumers:

• panic on a 0 IggyDuration:

  iggy/core/sdk/src/clients/consumer.rs

  Line 328 in ec456d2

  let mut timer = time::interval(interval.get_duration());

• and (not) sleeping with 0 on the background store offset at interval task loop. :

  iggy/core/sdk/src/clients/consumer.rs

  Line 496 in ec456d2

  _ = sleep(interval.get_duration()) => {}

producers:

• panic on a 0 IggyDuration:

  iggy/core/sdk/src/clients/producer.rs

  Line 265 in ec456d2

  timer.get_or_insert_with(|| tokio::time::interval(interval.get_duration()));

and connection lifecycle:

• continuous heartbeat spins:

  iggy/core/sdk/src/clients/client.rs

  Line 298 in f42f5ab

  sleep(heartbeat_interval.get_duration()).await

Clarification:

• The problem occurs if manually setting a IggyDuration equal to zero.
• No default path actually sets an IggyDuration with zero.
• panics occur for IggyDuration in retry_intervals not for heartbeat_intervals

Solution:

• Having IggyDuration falling to 0 for "unlimited", "disabled" and "none" is a little misleading.
• Since it's a common type there might be unobserved behavior in other places should we change it now.
• To avoid panics, I would prefer not checking in methods calls as done in the PR but enforcing the proposed guards on a type level e.g. by introducing a NonZeroIggyDuration.

@haubur

Thanks for the review.

I agree that using NonZeroIggyDuration is cleaner than checking in each method. My concern is that some of these are cross-field rules, so I am not sure a type can enforce them. For example, interval = 0 is still valid when max_retries is bounded. Changing the field type would also be a breaking change for the SDK API.

So I would prefer to keep the current approach. What do you think?

@ethanlin01x fair points.

• A NonZeroIggyDuration assumes that 0 intervals are actually undesirable, which I would argue makes sense. I think its reasonable to enforce waiting a bit before retrying (instead of rapid fire). You can still set the duration to some very low time period if you want that, but that's intended behavior than not implicit as it is now.
• It would be a breaking API change, true. If thats something we want at this point is a question to the maintainers I guess, because there is a release coming up this week AFAIK.

@haubur breaking changes are totally fine at this stage, as we're very close to the next release, with lots of breaking changes anyway :)
And yes, I do agree, there should be at least like a 1 ms wait before the retries (while on the other hand, no wait at all for data producer/consumer running as a busy loop is totally fine for specific use cases).

Thanks @spetz! So @ethanlin01x lets have a NonZeroIggyDuration for retries and heartbeats, while keeping the busy loop for AutoCommit on an interval = 0. Would you want to rewrite? Otherwise, I can also take this.

@haubur Thanks, I just rewrote the whole PR to introduce NonZeroIggyDuration

@haubur thanks, all four comments are addressed in 9f07b53 and 71f3484.

/ready